Skip to content

getRemoteUser() returns principal name #9211

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

sjrd218
Copy link
Contributor

@sjrd218 sjrd218 commented Nov 20, 2020

Returns the name of the authenticated principle instead of falling through to the toString() method which may render a string representation of the entire object rather than a username.

This behavior is helpful in OAuth2 and Saml2 configurations.

Closes #3357

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Nov 20, 2020
@sjrd218
Copy link
Contributor Author

sjrd218 commented Nov 24, 2020

Closing this PR. I can't run the integration tests on my computer. They always fail. Feel free to use the code if you wish. It takes too much time to try to submit PRs to this project and I can't justify spending any more time trying to make this work.

@sjrd218 sjrd218 closed this Nov 24, 2020
@jzheaux
Copy link
Contributor

jzheaux commented Nov 25, 2020

@sjrd218, I'm sorry things didn't work well for you. If you are still interested, I wonder if the slow-down you are experiencing is something that I can help with.

Would you be able to post details about the issues that you were having? For example, what were the integration test failures, and what was it that was taking too much time?

There might be solutions to both, and it would be helpful for others who experience the same issues that you did.

@jzheaux
Copy link
Contributor

jzheaux commented Dec 3, 2020

@sjrd218 after reviewing the PR, I believe it's still a valuable contribution. 9c373ef and a polish of f614a82 are now merged into master.

@jzheaux jzheaux self-assigned this Dec 3, 2020
@jzheaux jzheaux added in: web An issue in web modules (web, webmvc) type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Dec 3, 2020
@jzheaux jzheaux added this to the 5.5.0-M2 milestone Dec 3, 2020
@lnxmad
Copy link

lnxmad commented Jan 11, 2021

This is a showstopper for my team.. Any way to release this bugfix sooner than 5.5.0.M2?

@jzheaux
Copy link
Contributor

jzheaux commented Jan 11, 2021

Hi, @lnxmad. There are no releases planned before 5.5.0-M2.

Can your problem be addressed by using a custom principal whose toString outputs what you need? Or, you can introduce a custom filter that wraps the HttpServletRequest with a HttpServletRequestWrapper implementation that overrides the getRemoteUser method.

If neither of those work, then sharing more detail about your issue might help.

@lnxmad
Copy link

lnxmad commented Jan 11, 2021

I did consider a filter to wrap request, however AuthenticationTrustResolver is used and doesn't appear to be a bean to inject. I'd have to create a new instance (may not be an issue). As far as the principal object, I am using out of the box OIDC and haven't tracked down a good place to create new principal to extend/override the toString(). I can take another look at that.

Thanks for the quick response!

@jzheaux
Copy link
Contributor

jzheaux commented Jan 11, 2021

You might consider injecting a custom OidcUserService. Your custom one can delegate to the default one and then change out the principal, like so:

@Bean 
OAuth2UserService<OidcUserRequest, OidcUser> oidcUserService() {
    OidcUserService delegate = new OidcUserService(); // the default
    return (userRequest) -> {
        OidcUser oidcUser = delegate.loadUser(userRequest);
        return new MyCustomOidcUser(oidcUser);
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web An issue in web modules (web, webmvc) type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SEC-3198: SecurityContextHolderAwareRequestWrapper#getRemoteUser ignores Authenticaion#getName
4 participants